Fixing #7140: Implementing new lazy vals scheme #14545

olsdavis · 2022-02-23T10:04:33Z

Implementing the new lazy vals scheme mentioned in #6979, fixing #7140; note that, however:

DottyBytecodeTests.lazyFields does not pass anymore, as the new number of instructions generated by the lazy vals scheme is higher (126);
the prototype code had to be rewritten (namely, pattern matching has been rewritten in terms of if-else's), because the lazy vals transformation comes after certain others.

sjrd · 2022-02-23T10:14:46Z

/cc @WojciechMazur You may want to have a look at this, from the point of view of Scala Native.

WojciechMazur · 2022-02-23T11:16:12Z

@sjrd Thank you for the mention.
For some context, let me explain how we deal with lazy vals currently. In SN we don't have sun.misc.unsafe package at all, we also currently don't have some of the JVM Class runtime information (eg. clazz.declaredFields). What we currently did was collect the names of bitmap fields with their associated symbol. At any place of usage instead of using offset field with the value calculated using sun.misc.unsafe methods we were using our own intrinsic operation - given an object instance and name of the field we would calculate the exact pointer to memory location when generating our code to LLVM (at this point we know exact memory layout).
We would use this pointer is our own specific LazyVals helpers methods, taking ptr: Ptr[_], ... instead of t: Object, offset: Long, ... as defined here. We don't use offsets stored in the fields - at any place of usage we use our intrinsic operation which would be replaced with a constant value when linking.
The compiler phase responsible for adopting lazy vals can be found here

Overall I think that these changes would not be problematic in Scala Native, but still would need to adapt Lazy Vals in Scala Native compiler plugin, at least for now. The only thing that would change is the field we would calculate pointer off.
The main question I have is does this change fix problems with reflective access in Graal Native Image?

smarter · 2022-02-23T13:37:35Z

The main question I have is does this change fix problems with reflective access in Graal Native Image?

I don't think so, as far as I can tell this can't be fixed without requiring Java 9+ cf #9013

smarter · 2022-02-24T12:09:49Z

Because the runtime is different, I assume we'll need to keep using the old implementation when compiling with -Yscala-release 3.1 or anterior, wdyt @prolativ? (and can you help @olsdavis come up with tests involving -Yscala-release?)

sjrd · 2022-02-24T12:55:36Z

compiler/src/dotty/tools/dotc/transform/LazyVals.scala

+   *       if current.isInstanceOf[A] then
+   *         current.asInstanceOf[A]


This must come last. If A erases to Object, this test will be true for any non-null value, including instances of NULL, Waiting and Evaluating!

Also, it should mention the return explicitly in this variant:

Suggested change

* if current.isInstanceOf[A] then

* current.asInstanceOf[A]

* if current.isInstanceOf[A] then

* return current.asInstanceOf[A]

Same for return null below.

sjrd · 2022-02-24T12:55:52Z

compiler/src/dotty/tools/dotc/transform/LazyVals.scala

+   *    case current: A =>
+   *        current


Similarly, this must come last.

prolativ · 2022-02-24T14:42:44Z

@smarter right, if we extend the standard library, we cannot use these extensions when producing bytecode which is supposed to work with an older compiler. Also all the declarations of vals, classes etc. added to runtime/LazyVals.scala should have a @since annotation with the appropriate scala minor version, which I assume to be 3.2. Is this PR going to stay unmerged until the point we're sure the next version we are going to release is 3.2.0-RC1 then?

Regarding the tests the easiest way to find some examples of how this is done is just to search for files with names ending with _r3.0.scala. There's some additional documentation on that in docs/docs/contributing/testing.md but I'll be happy to help is anything is unclear.

nicolasstucki · 2022-03-14T14:33:28Z

Regarding the tests the easiest way to find some examples of how this is done is just to search for files with names ending with _r3.0.scala. There's some additional documentation on that in docs/docs/contributing/testing.md but I'll be happy to help is anything is unclear.

@olsdavis I could help you with this part if you don't know how to do it.

olsdavis · 2022-03-14T15:30:20Z

@nicolasstucki Yes, sure! Thank you

nicolasstucki · 2022-03-28T10:18:40Z

Regarding the tests the easiest way to find some examples of how this is done is just to search for files with names ending with _r3.0.scala. There's some additional documentation on that in docs/docs/contributing/testing.md but I'll be happy to help is anything is unclear.

@prolativ are we still missing a way to execute the run tests using the 3.0 library? Without this, we cannot easily test that this feature actually used the release flag and generated compatible code.

nicolasstucki · 2022-03-28T10:27:18Z

@olsdavis you can add the following tests files. These show that the current library is compatible with code generated with older versions of the compiler.

tests/run/lazyVals_c3.0.0.scala

// Compiled with 3.0.0 and run with current compiler
class Foo:
  lazy val x =
    println("computing x")
    "x"
  lazy val y =
    println("computing y")
    "y"

@main def Test =
  val foo = new Foo
  println(foo.x)
  println(foo.y)

tests/run/lazyVals_c3.0.0.check

computing x
x
computing y
y

tests/run/lazyVals_c3.1.0.scala

// Compiled with 3.1.0 and run with current compiler
class Foo:
  lazy val x =
    println("computing x")
    "x"
  lazy val y =
    println("computing y")
    "y"

@main def Test =
  val foo = new Foo
  println(foo.x)
  println(foo.y)

tests/run/lazyVals_c3.1.0.check

computing x
x
computing y
y

nicolasstucki

I think those are all the changes needed to support the release flag. I still have to figure out how to test the runtime when targeting an older release.

nicolasstucki · 2022-03-28T10:32:53Z

library/src/scala/runtime/LazyVals.scala

+      clz.getDeclaredFields.nn.foreach(println(_))
+    val field = clz.getDeclaredField(name)
+    if java.lang.reflect.Modifier.isStatic(field.nn.getModifiers()) then
+      unsafe.staticFieldOffset(field)


Not sure if this is compatible with code compiled with 3.0. Could would add a case where this would be needed in the new implementation to tests/run/lazyVals_c3.0.0.scala and tests/run/lazyVals_c3.1.0.scala?

If we have conflicts, we should create a variant of getOffset for the new encoding and keep the old one for backward compatibility.

This PR also adds getStaticOffset and the compiler should know which fields are static, so couldn't it emit calls to getStaticOffset when needed instead of checking this at runtime?

nicolasstucki · 2022-03-28T10:48:42Z

compiler/src/dotty/tools/dotc/transform/LazyVals.scala

+    val body = initBlock(ValDef(current, ref(target)) :: If(ref(current).equal(nullLiteral), unevaluated, ifNotNull) :: Nil)
+    val mainLoop = WhileDo(EmptyTree, body) // becomes: while (true) do { body }
+    val ret = DefDef(methodSymbol, initBlock(discardDef :: mainLoop :: Nil))
+    ret
  }

  def transformMemberDefThreadSafe(x: ValOrDefDef)(using Context): Thicket = {


When targetting an older version such as 3.0 we need to generate the old version of the code.
I suggest making two variants of transformMemberDefThreadSafe one with the old code and one with the new code. Do the same for mkThreadSafeDef. Then transformMemberDefThreadSafe can dispatch to the correct one as follows

import dotty.tools.dotc.config.ScalaRelease.* def transformMemberDefThreadSafe(x: ValOrDefDef)(using Context): Thicket = // TODO find more meaningful names, not old/new. if ctx.scalaRelease <= Release3_1 then transformMemberDefThreadSafeOld(x) else transformMemberDefThreadSafeNew(x)

Note: use Release3_0 as a starting point. Otherwise, you will always get the old version as the main is also on 3.1. This is something that we have to change when going to 3.2.

You can use a variant of lazyVals_c3.0.0.scala named lazyVals_r3.0.scala to have a test that sets the scala release flag to 3.0. This will compile to code with the current compiler but target the old stdlib. Unfortunately, the test will run with the current library. We still need to figure out the simplest way to run with the old library.

prolativ · 2022-03-28T11:47:11Z

@prolativ are we still missing a way to execute the run tests using the 3.0 library? Without this, we cannot easily test that this feature actually used the release flag and generated compatible code.

The _c3.0.2 suffix does that. This should currently work for any published 3.x.y version (scala 2 could also be supported with minimal effort).
Or do you mean the problems with using the latest release as scalaOutputVersion #14306?
Or maybe you meant the ability to use one version of the compiler to compile a file and another version to execute everything at runtime? This isn't currently supported as we didn't seem to have a proper use case for that.

nicolasstucki · 2022-03-28T12:26:15Z

Needs rebase on the current main branch

nicolasstucki · 2022-03-28T12:38:10Z

library/src/scala/runtime/LazyVals.scala

+   * Used to indicate the state of a lazy val that is being
+   * evaluated and of which other threads await the result.
+   */
+  final class Waiting:


All new definitions in this class should be added to

project/MiMaFilters.scala

ProblemFilters.exclude[DirectMissingMethodProblem]("scala.runtime.LazyVals.getStaticOffset"), ProblemFilters.exclude[DirectMissingMethodProblem]("scala.runtime.LazyVals.getStaticOffset"), ProblemFilters.exclude[DirectMissingMethodProblem]("scala.runtime.LazyVals.objCAS"), ProblemFilters.exclude[DirectMissingMethodProblem]("scala.runtime.LazyVals.objCAS"), ProblemFilters.exclude[DirectMissingMethodProblem]("scala.runtime.LazyVals#Names.evaluating"), ProblemFilters.exclude[DirectMissingMethodProblem]("scala.runtime.LazyVals#Names.getStaticOffset"), ProblemFilters.exclude[DirectMissingMethodProblem]("scala.runtime.LazyVals#Names.nullValued"), ProblemFilters.exclude[DirectMissingMethodProblem]("scala.runtime.LazyVals#Names.objCas"), ProblemFilters.exclude[DirectMissingMethodProblem]("scala.runtime.LazyVals#Names.waiting"), ProblemFilters.exclude[DirectMissingMethodProblem]("scala.runtime.LazyVals#Names.waitingAwaitRelease"), ProblemFilters.exclude[DirectMissingMethodProblem]("scala.runtime.LazyVals#Names.waitingRelease"), ProblemFilters.exclude[MissingClassProblem]("scala.runtime.LazyVals$Evaluating$"), ProblemFilters.exclude[MissingClassProblem]("scala.runtime.LazyVals$NULL$"), ProblemFilters.exclude[MissingClassProblem]("scala.runtime.LazyVals$Waiting"), ProblemFilters.exclude[MissingFieldProblem]("scala.runtime.LazyVals.Evaluating"), ProblemFilters.exclude[MissingFieldProblem]("scala.runtime.LazyVals.NULL"),

This can be tested with

sbt:scala3> reload;scala3-interfaces/mimaReportBinaryIssues;scala3-library-bootstrapped/mimaReportBinaryIssues

@SInCE

* Add missing @SInCE annotations to definitions * Add missing checks for @SInCE * Add more tests

* Make -scala-release an experimental setting * Add better checks of of TASTy version * Validate names of releases inside since annotations * Extend Vulpix to check actual counts and possitions of errors from other compilers

Fixes scala#14319

The previous check did not consider the case where the self constructor was itself a block that originated from named and default parameters in the actual self constructor call. That gave a false negative for some code in akka. The negative was not discovered before since we did not propagate the correct context information into the expression of a block.

When parsing a `TRY` if there is an empty catch block instead of just returning a syntax error return an incomplete if you're at the EOF. This ensures that in the REPL if you are in a position like: ```scala scala> try { | ??? | } catch ``` And you hit enter you'll still be able to continue. Fixes scala#4393

Fixes scala#14415

- Implements a propotype of the new lazy vals scheme Fix/Fixing/Fixes/Close/Closing/Refs scala#7140

szymon-rd · 2022-05-17T15:07:28Z

Closing and continuing work in #15207, comments will be addressed there.

Cleaned up version of #14545 Implementing the new lazy vals scheme mentioned in #6979, fixing #7140 New PR to avoid force pushing to main branch of the previous author. Fixes #15149 as well Applied #14780

sjrd reviewed Feb 24, 2022

View reviewed changes

smarter added the needs-minor-release This PR cannot be merged until the next minor release label Feb 24, 2022

dwijnand assigned sjrd Mar 2, 2022

smarter added this to the 3.2.0-RC1 milestone Mar 7, 2022

bishabosha assigned olsdavis and unassigned sjrd Mar 7, 2022

nicolasstucki suggested changes Mar 28, 2022

View reviewed changes

nicolasstucki reviewed Mar 28, 2022

View reviewed changes

anatoliykmetyuk mentioned this pull request Apr 4, 2022

Make lazy vals and object native image friendly #14780

Merged

Kordyjan and others added 10 commits April 4, 2022 20:00

Change -scala-release flag into proper VersionSetting

54a9d0e

Add since annotation

7b7498a

Add since annotations to all symbols added in 3.1

e535c85

Restrict scope of since annotation

d03c327

Check for types with since annotation

68017a3

wip

6e04111

Finalize work on @SInCE and -scala-release:

99080ed

* Add missing @SInCE annotations to definitions * Add missing checks for @SInCE * Add more tests

Reworks for forward compat:

e99a524

* Make -scala-release an experimental setting * Add better checks of of TASTy version * Validate names of releases inside since annotations * Extend Vulpix to check actual counts and possitions of errors from other compilers

Use a more portable way to download an old compiler for tests

f88395d

scala-release: Adjust MiMa filters; Revert changes to tasty module

3104401

noti0na1 and others added 23 commits April 4, 2022 20:02

Fix typos

4d6668a

Add restriction

a82dcda

Move relaxed context to a function

9b58d19

fix(scala#11202): explicitly handle TypeTree in syntax highlighting

1253563

Better positions on errorTermTree

b7ea842

Thread context through block in transforms correctly and efficiently

618d3e7

Fixes scala#14319

Protected case class constructor -> private apply

105036d

Set context when reporting summonInline errors

60fe22f

Don't skip override tests when overridden is AbsOverride

69ede37

Fixes scala#14415

Introduce generation config for Scaladoc

5bb036c

Add task generating Scala 3 language reference

6c8f21d

Edit unused imports

bdbc32e

Remove unused definitions

d0bd3ba

Implementing lazy vals

76a1495

- Implements a propotype of the new lazy vals scheme Fix/Fixing/Fixes/Close/Closing/Refs scala#7140

Reorder to avoid type erasure to Object issue + fix test

20b5f0f

Keeping code for old release compatibility

6f1a43e

Fix conflicts

e4b37ba

Merge branch 'main' into master

e9deb26

fix null type

6337421

Merge branch 'master' of github.com:olsdavis/dotty

8aa44a8

fix nn for union type

ceb27ff

timzaak mentioned this pull request May 7, 2022

Scala 3 timzaak/blog#51

Closed

This was referenced May 12, 2022

@transient lazy val does not get marked as ACC_TRANSIENT #15149

Closed

New lazy vals implementation #15207

Closed

szymon-rd closed this May 17, 2022

szymon-rd mentioned this pull request May 26, 2022

New lazy vals implementation #15296

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing #7140: Implementing new lazy vals scheme #14545

Fixing #7140: Implementing new lazy vals scheme #14545

olsdavis commented Feb 23, 2022

sjrd commented Feb 23, 2022

WojciechMazur commented Feb 23, 2022 •

edited

Loading

smarter commented Feb 23, 2022

smarter commented Feb 24, 2022

sjrd Feb 24, 2022

sjrd Feb 24, 2022

sjrd Feb 24, 2022

prolativ commented Feb 24, 2022

nicolasstucki commented Mar 14, 2022

olsdavis commented Mar 14, 2022

nicolasstucki commented Mar 28, 2022

nicolasstucki commented Mar 28, 2022

nicolasstucki left a comment

nicolasstucki Mar 28, 2022

nicolasstucki Mar 28, 2022

smarter Mar 28, 2022

nicolasstucki Mar 28, 2022

nicolasstucki Mar 28, 2022

prolativ commented Mar 28, 2022

nicolasstucki commented Mar 28, 2022

nicolasstucki Mar 28, 2022

nicolasstucki Mar 28, 2022

szymon-rd commented May 17, 2022

Fixing #7140: Implementing new lazy vals scheme #14545

Fixing #7140: Implementing new lazy vals scheme #14545

Conversation

olsdavis commented Feb 23, 2022

sjrd commented Feb 23, 2022

WojciechMazur commented Feb 23, 2022 • edited Loading

smarter commented Feb 23, 2022

smarter commented Feb 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prolativ commented Feb 24, 2022

nicolasstucki commented Mar 14, 2022

olsdavis commented Mar 14, 2022

nicolasstucki commented Mar 28, 2022

nicolasstucki commented Mar 28, 2022

nicolasstucki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prolativ commented Mar 28, 2022

nicolasstucki commented Mar 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szymon-rd commented May 17, 2022

WojciechMazur commented Feb 23, 2022 •

edited

Loading